Test the jumpstarter-driver-ssh identity key injection#689
Test the jumpstarter-driver-ssh identity key injection#689kirkbrauer merged 2 commits intojumpstarter-dev:mainfrom
Conversation
WalkthroughAdds comprehensive tests in driver_test.py for SSH identity handling, covering identity strings, identity files, error cases, command construction, temporary key file creation/cleanup, and logging behaviors using a shared TEST_SSH_KEY constant. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor T as Test
participant W as SSHWrapper
participant TF as TempFile
participant S as SSH Client/Process
participant L as Logger
T->>W: execute(cmd, host, user, options)
alt identity string provided
W->>TF: create temp key file (0600)
TF-->>W: temp path or OSError
alt TF error
W->>L: warn/error (creation failed)
W-->>T: raise error
else success
W->>S: ssh -i <temp> -l user host cmd
S-->>W: return code/output
W->>TF: cleanup
alt cleanup fails
W->>L: warn (cleanup failed)
end
W-->>T: result
end
else identity file provided
W->>S: ssh -i <file> -l user host cmd
S-->>W: result
W-->>T: result
else no identity
W->>S: ssh -l user host cmd
S-->>W: result
W-->>T: result
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
mangelajo
force-pushed
the
test-ssh-identity
branch
from
7290260 to
eebfc02
Compare
mangelajo
force-pushed
the
test-ssh-identity
branch
from
eebfc02 to
6fccabc
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py (2)
450-451: Ensure cross-platform temp path assertions.The assertions for temp file paths assume Unix-like systems (
/tmpor/var/tmp). On Windows, temp paths are typically inC:\Users\...\AppData\Local\Temp.Apply this diff to make the assertion platform-independent:
+ import tempfile # The identity file should be a temporary file assert identity_file_path.endswith("_ssh_key") - assert "/tmp" in identity_file_path or "/var/tmp" in identity_file_path + assert tempfile.gettempdir() in identity_file_path
498-500: Ensure cross-platform temp path assertions.Same issue as in
test_ssh_command_with_identity_string: assertions assume Unix-like paths.Apply this diff:
+ import tempfile as tmp_module # The identity file should be a temporary file (not the original file) assert identity_file_path.endswith("_ssh_key") - assert "/tmp" in identity_file_path or "/var/tmp" in identity_file_path + assert tmp_module.gettempdir() in identity_file_path assert identity_file_path != temp_file_path
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py (3)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver.py (2)
SSHWrapper(9-51)client(35-36)packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py (1)
TcpNetwork(88-121)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (1)
run(45-75)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: check-warnings
- GitHub Check: build
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: e2e
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
🔇 Additional comments (7)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py (7)
13-18: LGTM: Shared test constant improves maintainability.The
TEST_SSH_KEYconstant centralizes the test SSH key content, making it easy to update if needed and ensuring consistency across all tests that use it.
360-374: LGTM: Validates identity string configuration.The test correctly verifies that
ssh_identityis set when provided as a string and thatssh_identity_fileremainsNone.
404-412: LGTM: Validates mutual exclusivity of identity options.The test correctly verifies that providing both
ssh_identityandssh_identity_fileraises aConfigurationError.
415-422: LGTM: Validates read error handling.The test correctly verifies that attempting to read a nonexistent identity file raises a
ConfigurationErrorwith an appropriate error message.
516-546: LGTM: Validates absence of identity flag.The test correctly verifies that when no identity is provided, the
-iflag is not included in the SSH command.
587-609: LGTM: Validates temp file creation error handling.The test correctly verifies that an
OSErrorduring temporary file creation is properly propagated within anExceptionGroupwrapper.
611-649: LGTM: Validates graceful cleanup failure handling.The test correctly verifies that when temporary file cleanup fails, a warning is logged and the SSH command execution still succeeds.
| import tempfile | ||
|
|
||
| # Create a temporary file with SSH key content | ||
| with tempfile.NamedTemporaryFile(mode='w', delete=False, suffix='_test_key') as temp_file: |
There was a problem hiding this comment.
Consider using binary mode for cross-platform consistency.
The PR description mentions opening files in "wb" mode to avoid CRLF format issues on Windows. This test uses text mode ('w'), which may introduce line ending conversions on Windows.
Apply this diff to align with the PR objectives:
- with tempfile.NamedTemporaryFile(mode='w', delete=False, suffix='_test_key') as temp_file:
+ with tempfile.NamedTemporaryFile(mode='wb', delete=False, suffix='_test_key') as temp_file:And update the write call:
- temp_file.write(TEST_SSH_KEY)
+ temp_file.write(TEST_SSH_KEY.encode('utf-8'))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| with tempfile.NamedTemporaryFile(mode='w', delete=False, suffix='_test_key') as temp_file: | |
| # write the SSH key in binary mode to avoid unwanted CRLF conversions on Windows | |
| - with tempfile.NamedTemporaryFile(mode='w', delete=False, suffix='_test_key') as temp_file: | |
| with tempfile.NamedTemporaryFile(mode='wb', delete=False, suffix='_test_key') as temp_file: | |
| temp_file.write(TEST_SSH_KEY.encode('utf-8')) |
🤖 Prompt for AI Agents
In packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py around
line 382, the temp file is opened in text mode ('w') which can cause CRLF
conversions on Windows; change the NamedTemporaryFile mode to 'wb' and update
the subsequent write call to write bytes (e.g., b'...') so the file is written
in binary with consistent line endings across platforms.
| import tempfile | ||
|
|
||
| # Create a temporary file with SSH key content | ||
| with tempfile.NamedTemporaryFile(mode='w', delete=False, suffix='_test_key') as temp_file: |
There was a problem hiding this comment.
Consider using binary mode for cross-platform consistency.
Same issue as in test_ssh_identity_file_configuration: using text mode ('w') may introduce line ending conversions on Windows.
Apply this diff:
- with tempfile.NamedTemporaryFile(mode='w', delete=False, suffix='_test_key') as temp_file:
+ with tempfile.NamedTemporaryFile(mode='wb', delete=False, suffix='_test_key') as temp_file:And update the write call:
- temp_file.write(TEST_SSH_KEY)
+ temp_file.write(TEST_SSH_KEY.encode('utf-8'))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| with tempfile.NamedTemporaryFile(mode='w', delete=False, suffix='_test_key') as temp_file: | |
| with tempfile.NamedTemporaryFile(mode='wb', delete=False, suffix='_test_key') as temp_file: | |
| temp_file.write(TEST_SSH_KEY.encode('utf-8')) |
🤖 Prompt for AI Agents
In packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py around
line 470, the temp file is opened in text mode ('w') which can cause CRLF
conversions on Windows; change the NamedTemporaryFile mode to binary ('wb') and
update the subsequent write call to write bytes (encode the string or use a
bytes literal) so the key is written verbatim across platforms.
| result = client.run(False, ["hostname"]) | ||
|
|
||
| # Verify temporary file was created | ||
| mock_temp_file.assert_called_once_with(mode='w', delete=False, suffix='_ssh_key') |
There was a problem hiding this comment.
Update assertion to expect binary mode.
The test assertion expects mode='w', but per the PR objectives, the implementation should use mode='wb' to avoid CRLF issues on Windows.
Apply this diff:
- mock_temp_file.assert_called_once_with(mode='w', delete=False, suffix='_ssh_key')
+ mock_temp_file.assert_called_once_with(mode='wb', delete=False, suffix='_ssh_key')Additionally, update the mock to encode the SSH key:
- mock_temp_file_instance.write.assert_called_once_with(TEST_SSH_KEY)
+ mock_temp_file_instance.write.assert_called_once_with(TEST_SSH_KEY.encode('utf-8'))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| mock_temp_file.assert_called_once_with(mode='w', delete=False, suffix='_ssh_key') | |
| # … earlier in test_ssh_identity_temp_file_creation_and_cleanup … | |
| # Expect the temp file to be opened in binary mode now | |
| mock_temp_file.assert_called_once_with(mode='wb', delete=False, suffix='_ssh_key') | |
| # Ensure we write bytes, not str | |
| mock_temp_file_instance.write.assert_called_once_with(TEST_SSH_KEY.encode('utf-8')) | |
| # … following assertions (chmod, unlink, etc.) … |
🤖 Prompt for AI Agents
In packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py around
line 574, the assertion currently expects mode='w' but the implementation should
open the temp file in binary mode; change the assertion to expect mode='wb' and
update any mock file-like object or return value used for the temp file so that
written SSH key data is provided/compared as bytes (i.e., encode the SSH key
string before asserting writes or side effects). Ensure the
mock_temp_file.assert_called_once_with uses mode='wb', delete=False,
suffix='_ssh_key' and adjust any subsequent mock write/read checks to use byte
strings.
|
the keys are text, so no need to add "b" |
|
LGTM |
(cherry picked from commit 0f1555f)
3 participants
Follow up to #687
Summary by CodeRabbit